Skip to content

Custom test checks #136

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 4 commits into from

Conversation

YannickJadoul
Copy link
Member

Cfr. #103 (after having come up in #135 again)

For each integration test, check.py is now called instead of asserting the number of wheels present. To reduce code duplication, I've also implemented a solution to #105.

@YannickJadoul YannickJadoul force-pushed the custom-test-checks branch 5 times, most recently from 272312f to 3ac0c60 Compare April 20, 2019 18:32
@YannickJadoul
Copy link
Member Author

Finally, seems I got it working :-)

Copy link
Contributor

@joerick joerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for getting the ball rolling here! Would you mind if I made a few changes to this @YannickJadoul? I want to try rolling the environment.json file into the test runner script, too.

import subprocess
import sys

build_identifiers = subprocess.check_output([sys.executable, '-m', 'cibuildwheel', '--print-build-identifiers'], universal_newlines=True).strip().split('\n')
Copy link
Contributor

@joerick joerick Apr 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure on getting cibuildwheel to tell us what it should be producing... a mistake in the python configurations would then not be caught by the tests. Also, I'm happy for the tests to contain extra, redundant information, as long as that is checked by the machine! So I think I'd prefer the tests to assert all the wheels that they expect to see on each platform.

The --print-build-identifiers arg will be useful regardless :).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm. Maybe on reflection, just one test needs to check that the build identifiers match the produced wheels. Then we can trust it after that.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure on getting cibuildwheel to tell us what it should be producing... a mistake in the python configurations would then not be caught by the tests.

Good point; the using this --print-build-identifiers still has turned out to be more code duplication than expected. It would be good if we could make some utilities Python code to be shared by the different tests.

The --print-build-identifiers arg will be useful regardless :).

If we can do the previous, should I then split up this PR into two separate ones? One for the tests, and one for --print-build-identifiers?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe on reflection, just one test needs to check that the build identifiers match the produced wheels. Then we can trust it after that.

Problem here is that there's no unambiguous mapping from build identifier to wheel name. Particularly in the cp27-cp27m and cp27-cp27mu cases, which both have cp27-... identifiers.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Problem here is that there's no unambiguous mapping from build identifier to wheel name. Particularly in the cp27-cp27m and cp27-cp27mu cases, which both have cp27-... identifiers

argh, good point. maybe just statically writing out expected wheels in the test will be better then.

If we can do the previous, should I then split up this PR into two separate ones? One for the tests, and one for --print-build-identifiers?

I think for the later tests might find the build_identifiers useful, let's keep it in for now

@YannickJadoul
Copy link
Member Author

YannickJadoul commented Apr 22, 2019

Would you mind if I made a few changes to this?

Sure, of course. This was just the first I could come up with.

EDIT:

I want to try rolling the environment.json file into the test runner script, too.

What do you have in mind? I was trying to avoid the individual script per test to 1) run cibuildwheel manually, and 2) reproduce the environment manipulation.

@joerick
Copy link
Contributor

joerick commented Apr 22, 2019

What do you have in mind? I was trying to avoid the individual script per test to 1) run cibuildwheel manually, and 2) reproduce the environment manipulation.

Check out #137 for another option – it's just got a couple advantages like being able to skip tests on platforms that aren't relevant.

There is a bit more code duplication, but I think it's worth it to be able to be more explicit in the tests.

@YannickJadoul YannickJadoul deleted the custom-test-checks branch April 25, 2019 15:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants